Skip to content

Add extra_returning_fields in apply#767

Open
kigawas wants to merge 1 commit into
python-gino:masterfrom
kigawas:extra-returning-fields
Open

Add extra_returning_fields in apply#767
kigawas wants to merge 1 commit into
python-gino:masterfrom
kigawas:extra-returning-fields

Conversation

@kigawas

@kigawas kigawas commented Mar 17, 2021

Copy link
Copy Markdown
Contributor

Closes #730

@kigawas kigawas force-pushed the extra-returning-fields branch 3 times, most recently from b3be96a to 24aa060 Compare March 17, 2021 08:11
@kigawas kigawas force-pushed the extra-returning-fields branch from 24aa060 to c224408 Compare March 18, 2021 06:37
@xnuinside

xnuinside commented May 3, 2021

Copy link
Copy Markdown
Contributor

@kigawas, hi! Good PR & additional feature.

@wwwjfy, @fantix, what do you think?

@wwwjfy

wwwjfy commented Jun 20, 2021

Copy link
Copy Markdown
Member

Sorry for the late.

I'm thinking of a parameter to return/reload every column instead of specified ones only. It saves a get call, which is probably being used in such case now.

Another nitpicky point is the usage of tuple in this PR. As I understand, list is more suitable here, semantically. Quoting Python doc: Tuples are immutable sequences, typically used to store collections of heterogeneous data

@kigawas

kigawas commented Jun 20, 2021

Copy link
Copy Markdown
Contributor Author

Yes. Tuple should be replaced by Sequence in Python, which sugguests an immutable sequence although you can still pass list.

I'm thinking of a parameter to return/reload every column instead of specified ones only. It saves a get call, which is probably being used in such case now.

It's certainly better to get all columns by default, but for now a transitional approach would be more realistic to avoid API change

@wwwjfy

wwwjfy commented Jun 20, 2021

Copy link
Copy Markdown
Member

I don't mean that big a change, but just an additional parameter like update_all (need a good name) to return all columns and update the instance. Similar to the one in the PR, but no need to list every single one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extra returning fields in UpdateRequest.apply

3 participants